-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Opt in to remaining Rails 7.1 defaults #30332
base: main
Are you sure you want to change the base?
Conversation
@@ -57,9 +57,7 @@ | |||
module Mastodon | |||
class Application < Rails::Application | |||
# Initialize configuration defaults for originally generated Rails version. | |||
config.load_defaults 7.0 | |||
|
|||
config.active_record.marshalling_format_version = 7.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was previously set here to work around a config bug where it didn't get set correctly from the new_framework_defaults file. With bump to 7.1 defaults, it isn't needed.
# in 7.0), then you need to configure SHA-256 for Active Record Encryption: | ||
# Rails.application.config.active_record.encryption.hash_digest_class = OpenSSL::Digest::SHA256 | ||
# | ||
# If you don't currently have data encrypted with Active Record encryption, you can disable this setting to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are effectively in this scenario -- we have the recently enabled otp AR encryption column, but that has only existed on 7.1 and has never been in a release, so I think we are in the SHA256 case described above, and having the framework default of not backwards-supporting SHA1 is acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought that as well, but:
May 31 14:59:06 mastodon bundle[993665]: E, [2024-05-31T14:59:06.438057 #993665] ERROR -- : [64fdf096-f096-476a-a51b-9cc20e91919a]
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] ActiveRecord::Encryption::Errors::Decryption (ActiveRecord::Encryption::Errors::Decryption):
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a]
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] app/controllers/concerns/auth/two_factor_authentication_concern.rb:32:in `valid_otp_attempt?'
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] app/controllers/concerns/auth/two_factor_authentication_concern.rb:74:in `authenticate_with_two_factor_via_otp'
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] app/controllers/concerns/auth/two_factor_authentication_concern.rb:51:in `authenticate_with_two_factor'
May 31 14:59:06 mastodon bundle[993665]: [64fdf096-f096-476a-a51b-9cc20e91919a] lib/mastodon/rack_middleware.rb:9:in `call'
I'm not completely sure this is because of this particular setting, though.
# replicas will not be able to deserialize `BigDecimal` arguments from this | ||
# serializer. Therefore, this setting should only be enabled after all replicas | ||
# have been successfully upgraded to Rails 7.1. | ||
# Rails.application.config.active_job.use_big_decimal_serializer = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one needs consideration as to what the impact would be of enabling now -vs- waiting for a proper 4.3 release running Rails 7.1 first (and then enable this in a future release).
# | ||
# In Rails 7.1, the new default is `:json_allow_marshal` which serializes and | ||
# deserializes with `ActiveSupport::JSON`, but can fall back to deserializing | ||
# with `Marshal` so that legacy messages can still be read. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fallback behavior helps us.
# | ||
# If you are performing a rolling deploy of a Rails 7.1 upgrade, wherein servers | ||
# that have not yet been upgraded must be able to read messages from upgraded | ||
# servers, first deploy without changing the serializer, then set the serializer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another one where we may want a 4.3 release running rails 7.1 first, and then make this change after.
# not yet been upgraded must be able to read messages from upgraded servers, | ||
# leave this optimization off on the first deploy, then enable it on a | ||
# subsequent deploy. | ||
# Rails.application.config.active_support.use_message_serializer_for_metadata = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another one - maybe should wait for post-7.1 release.
57ed14c
to
dacd7e4
Compare
dacd7e4
to
9ec35cf
Compare
I believe that the remainder of the options which were not already enabled here (in the file deleted in this PR) are safe to enable at this point, and we can flip over to the 7.1 defaults.
However - I know we also want to be deliberate with how people step through upgrades, and we may want the 4.3 release to include an update to Rails 7.1 (4.2.x is on Rails 7.0) but NOT immediately enable all these defaults.
Vaguely related previous PRs:
bin/rails app:update
with Rails 7.1 #27522Opening this to step through the final remaining changes in this diff, and coordinate the interaction of release timing/versions with when to make this change.